feat(jira): include Jira attachments and recent comments in task context (#577)#619
Conversation
…ext (aws-samples#577) Jira-origin tasks previously saw only the issue summary + description (ADF→ markdown) plus embedded HTTPS image URLs. Jira-hosted `media` file attachments and issue comments were invisible to the agent, so a ticket that says "see the attached log" or carries acceptance clarifications in comments would run under-informed. Bring Jira to parity with Linear's on-demand context reads by fetching the context authenticated at task-admission time in the webhook processor (the Atlassian Remote MCP can't run headlessly — see aws-samples#580). - agent-admission download path (cdk/src/handlers/shared/jira-attachments.ts): * downloadScreenAndStoreJiraAttachments — resolve Jira `media` attachments via the gateway attachment-content endpoint (api.atlassian.com/ex/jira/{cloudId}, the only host the 3LO token is valid against), refresh-and-retry-once on 401/403, enforce the size cap while streaming, validate magic bytes, screen through the existing Bedrock Guardrail, upload cleaned bytes to S3, and return `passed` AttachmentRecords. Fail-closed: a selected attachment that can't be safely fetched/screened throws JiraAttachmentError and the task is rejected with a Jira comment. Unsupported/oversized/over-cap attachments are silently skipped. Filenames are sanitized and ids validated to a safe token so neither can traverse the S3 key / agent on-disk path. * fetchRecentHumanComments — recent human (accountType `atlassian`) comments, ADF→markdown, oldest-first. Fail-open: any failure proceeds without them. - create-task-core: TaskCreationContext gains optional `taskId` (so processor- uploaded S3 keys match the eventual record) and `preScreenedAttachments` (merged verbatim, never re-screened; a non-`passed` record fails closed). - jira-webhook-processor: wire both in; comments fold under a "## Recent comments" heading, bounded so they never push task_description past the 10k limit (advisory context must not become a hard gate). - jira-adf.ts: extract the ADF→markdown walker so the processor and the comment helper share it without a circular import (behavior-preserving). - CDK: pass the attachments bucket to JiraIntegration; grant the processor ReadWrite + ATTACHMENTS_BUCKET_NAME env; bump the async processor timeout to 60s for serial download+screen. No new OAuth scopes (read:jira-work covers it). - docs: JIRA_SETUP_GUIDE "Issue context: attachments and comments" (supported types, limits, skip vs fail-closed reject, comment behavior) + Starlight mirror. Tests: new jira-attachments.test.ts (download/filter/screen/upload, sanitize, 401 retry, size cap, comment human/app filter + fail-open); processor tests for comment folding + truncation and fail-closed attachment rejection; create-task- core tests for preScreenedAttachments merge + taskId. `mise run build` green.
…s-samples#577) Jira issue webhook payloads that carry attachment metadata exceed the AWS managed `SizeRestrictions_BODY` 8 KB limit, so WAF returned 403 at the edge and the webhook receiver Lambda never ran — the delivery vanished before ABCA saw it. This is exactly the case aws-samples#577 depends on (an issue with file attachments), so without this exemption the feature can't work in any WAF-protected deploy. Mirror the existing `/v1/linear/webhook` and `/v1/github/webhook` handling — both rules must be updated together: - AWSManagedRulesCommonRuleSet-TaskPaths (excludes SizeRestrictions_BODY): add `/v1/jira/webhook` to the orStatement scope-down so large Jira bodies pass. - AWSManagedRulesCommonRuleSet (full CRS for other paths): add a NOT `/v1/jira/webhook` clause so the path isn't re-covered by the strict rule. The receiver still HMAC-verifies the raw body and the priority-4 rate-limit rule still applies, so relaxing the body-size check on this path is safe. Test: task-api.test.ts asserts the exclusion scope-down contains the Jira path and the full-CRS scope-down excludes it. 42 pass.
…amples#577) The Jira attachment-content endpoint (`/rest/api/3/attachment/content/{id}`) serves the file's own media type and responds 406 Not Acceptable to a narrow `Accept: application/octet-stream`. That made every attachment download fail with HTTP 406, and the fail-closed path then rejected the whole task ("could not be downloaded (HTTP 406)"). Send `Accept: */*` so the gateway serves the real content type. Verified against the live endpoint: octet-stream → 406, */* → 200. Test asserts the download request sets Accept: */*.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
=======================================
Coverage ? 89.43%
=======================================
Files ? 228
Lines ? 55898
Branches ? 5668
=======================================
Hits ? 49990
Misses ? 5908
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Review — Jira attachments + comments (#577)
Strong PR: correct reuse of the screening pipeline (incl. PDF), deliberate fail-closed/fail-open split, thorough path-traversal defenses, 181/181 tests green. Reviewed by reading + running the code (three independent adversarial passes, each finding re-verified). Below is everything actionable, ranked. The design is sound — these are edges, not a rework.
Verified non-issues (checked, no action): SSRF/bearer-token leak on redirect (undici drops Authorization cross-origin; host is a hardcoded constant), path traversal via id/filename, WAF exemption (HMAC verified before processing, rate-limit still covers the path, 10 MB API-GW ceiling), remainingSlots clamp, comment-budget off-by-one (lands exactly at 10k chars), non-passed fail-closed guard, magic-byte validation on real bytes.
Fix before merge
-
1. Empty attachment crashes the processor with no user feedback —
jira-attachments.ts:448→types.ts:~630. A 0-bytetext/plain/.log/.csv(ordinary user mistake) passes magic-bytes + screening →size_bytes: 0→createAttachmentRecord's!params.size_bytesguard throws a plainError(notJiraAttachmentError), outside the fail-closed conversion. The processor catches onlyJiraAttachmentError(jira-webhook-processor.ts:232), so the handler throws → no task, no ❌ comment (breaks the fail-closed contract), 2 async retries re-screen everything. Reproduced 3×.
Fix: increateAttachmentRecordguardparams.size_bytes === undefinedinstead of falsy (same for the size check), OR reject a zero-byte body asJiraAttachmentErrorbefore the factory. Add asize_bytes: 0regression test. -
2. 50 MB total-attachment cap trusts attacker metadata —
jira-attachments.ts:229.selectAttachmentssums the running total from payloadatt.size; the per-file 10 MB cap is stream-enforced but the total is never re-checked against real bytes.size: 1(or omitted) on 10 files → all pass → up to 100 MB actually downloaded/screened/stored vs the documented 50 MB (cost/limit-integrity, not memory — buffers are loop-local). Confirmed by running: ~10-byte declared batch pulled 60 MB.
Fix: track a real-bytes running total in the download loop; throw/stop once cumulative bytes exceedMAX_TOTAL_ATTACHMENT_SIZE_BYTES. -
3. S3 objects orphaned on any failure after upload —
jira-attachments.tshas no cleanup, andcreateTaskCore'scleanupOrphanedAttachmentsonly tracks inline uploads inuploadedS3Keys— the pre-screened Jira keys are never added. So a mid-batch throw, the non-passed500 guard, and a non-201 return (e.g. #4 below) all leave uploaded objects behind (90-day lifecycle is the only backstop). Diverges from the cleanup pattern this same PR relies on.
Fix: track uploaded keys in the download loop and delete them on throw; and/or have the processor delete the returnedpreScreenedAttachmentskeys whencreateTaskCorereturns non-201. Add a test asserting no orphan on a mid-batch failure.
Decision needed
- 4. A policy-tripping human COMMENT fails the whole task closed —
jira-webhook-processor.ts:466folds comments intotask_description, whichcreateTaskCoreguardrail-screens as one blob (create-task-core.ts:387);GUARDRAIL_INTERVENED→ 400 → task rejected. The PR's length-truncation budget keeps comments from gating on length, but content-gating comes back through the filter — a third-party comment with a PII-shaped string ("SSN 123-45-6789") or injection-like phrasing fails the reporter's task for text they didn't write. Proven end-to-end. (Description-screening pre-exists onmain; #577's new part is routing 3rd-party comment content into it.)
Options: screen the comment section separately and drop it on intervention (fail-open, matching the comment design), rather than letting it gate task creation — or accept + document the behavior.
Follow-up (robustness)
- 5. Async retries create duplicate tasks + duplicate attachment work — fresh
taskId = ulid()per invocation + noidempotencyKey(jira-webhook-processor.ts:407,479) meansattribute_not_exists(task_id)can't dedupe a redelivery. Costlier now (up to 10 re-downloads/screens/uploads per duplicate). Consider a deterministic idempotency key (e.g.issueKey#webhookTimestamp, matching the receiver's dedup key). - 6. 60s timeout is optimistic for 10 serial attachments — 10 × 10s fetch timeouts alone exceed 60s; a mid-loop kill orphans objects + triggers the #5 retry. Consider parallelizing the download/screen loop or raising the ceiling.
(This consolidates my two earlier comments on this PR into one list.)
… cap, comment screening, idempotency (aws-samples#577) Addresses isadeks's review on PR aws-samples#619: 1. Zero-byte attachment → JiraAttachmentError (fail-closed). A 0-byte body passed magic-byte + screening then tripped createAttachmentRecord's size guard with a plain Error outside the fail-closed conversion (no ❌ comment). Now rejected explicitly with a Jira comment. 2. 50 MB total-attachment cap enforced on REAL downloaded bytes, not the attacker-declared `size` metadata (which can under-report). Cumulative real bytes tracked in the download loop; throws once over the ceiling. 3. S3 orphan cleanup. downloadScreenAndStoreJiraAttachments now tracks uploaded keys and best-effort-deletes them if the batch throws mid-way; the processor deletes the returned pre-screened objects when createTaskCore returns non-201 (and on a 200 idempotent replay, whose objects the replayed task doesn't reference). New exported cleanupPreScreenedAttachments helper. 4. Comment content screened SEPARATELY and dropped fail-open. Third-party comment text that trips the guardrail no longer fails the reporter's task — screenCommentsOrDrop screens the folded comment block and drops it (task proceeds) on GUARDRAIL_INTERVENED / screening outage / no guardrail. 5. Deterministic idempotency key (`jira-<issueKey>-<webhookTimestamp>`) so an async webhook re-delivery dedupes instead of minting a duplicate task and re-downloading every attachment. A 200 idempotent replay is handled as success (no ❌ comment), cleaning up the round's re-uploaded objects. 6. Webhook-processor timeout 60s → 300s. 10 serial 10s attachment fetches can sum past 60s; a mid-loop kill would orphan objects and force a retry. Tests: +5 (zero-byte reject, real-byte total cap, mid-batch orphan cleanup, comment-blocked fail-open drop, deterministic idempotency key + 200-replay). mise run build green (cdk 2362).
…' into feat/577-pr
|
Thanks for the thorough review, @isadeks — all six addressed in the latest push. Summary: Fix before merge
Decision needed Follow-up Verified non-issues confirmed — thanks for checking those. |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Re-review of 1a742144 — all six addressed, verified by running
Checked out the head and re-ran my original repro scenarios against the fixed code (not just the new tests). Touched suites 144/144 green, tsc clean. All six confirmed:
Fix before merge
- Empty attachment ✓ — a 0-byte body now throws
JiraAttachmentErrorbeforecreateAttachmentRecord(jira-attachments.ts:406), so it routes through fail-closed reject-with-comment. Re-ran my probe:threw=JiraAttachmentError isJira=true(was a plainErrorcrashing the handler). - Real-byte total cap ✓ — cumulative
totalByteschecked againstMAX_TOTAL_ATTACHMENT_SIZE_BYTESon downloaded bytes (:395), not declaredsize. Re-ran the under-declared batch (6×9 MB real,size:1each): throws after 5 uploads (45 MB, before the 6th crosses 50 MB) — the 60 MB leak is closed. - S3 orphan cleanup ✓ — loop tracks
uploadedKeys, best-effortdeleteS3Objectson throw (:496-501); processor calls the new exportedcleanupPreScreenedAttachmentson both the non-201 path and the 200 idempotent-replay path. Re-ran the mid-batch-block probe: earlier uploadattachments/u/t/a0/f0.logis deleted (was orphaned).
Decision needed
4. Comment gating ✓ — went fail-open: screenCommentsOrDrop screens the comment section separately and returns [] (drops, task proceeds) on GUARDRAIL_INTERVENED, no-guardrail, or screening outage. Third-party comment content can no longer fail the reporter's task. (Coarse — one blocked line drops all comments — but that's the safe/advisory choice, not a defect.)
Follow-up
5. Duplicate-task retries ✓ — deterministic buildIdempotencyKey('jira-<issueKey>-<timestamp>'), sanitized to the valid charset + length-capped; I confirmed realistic keys (jira-KAN-42-…) pass isValidIdempotencyKey, so no risk of a malformed key 400-ing every Jira task. 200 replay handled as success (no ❌ comment) with cleanup.
6. Timeout ✓ — 30s→300s, with a note to parallelize the loop later.
Verified non-issues from my earlier review (SSRF/token-leak, path traversal, WAF exemption) still hold.
One thing NOT caused by this PR — but it's a required check and currently red
Secrets, deps, and workflow scan is failing, but it's osv-scanner flagging pre-existing transitive-dep vulns (astro 7.0.5, brace-expansion 1.1.15) in yarn.lock. This PR doesn't touch yarn.lock/package.json, and those advisories are already present on main — so it's dep-drift the scan newly caught, not from #577. Since it's a required merge gate, it'll block any open PR until someone bumps those deps (org-level chore). Flagging so it's not mistaken for a #577 regression and so the merge isn't held on Ayush to fix.
Everything I raised is genuinely fixed. LGTM once the dep-scan (unrelated) is sorted org-side.
…tern, not confirm-uploads Verified: PR aws-samples#619 (merged) already solved headless vendor attachments for Jira by fetching them AUTHENTICATED at task-admission time in the webhook processor (jira-attachments.ts) and injecting via create-task-core's preScreenedAttachments seam — bypassing the confirm-uploads/Cognito flow entirely. Also verified the confirm-uploads endpoint IS Cognito-only (task-api.ts:834), which is why the presigned/large-file path is unavailable to webhook callers — but aws-samples#619 shows that path is not needed. So the fix is NOT adding HMAC auth to confirm-uploads; it's building the Linear analog linear-attachments.ts (fetch uploads.linear.app + paperclip attachments with the @bgagent ChannelCredential, screen via Bedrock Guardrail, inject as preScreenedAttachments), mirroring aws-samples#619. P4 updated to reference aws-samples#619 as the template and to note the confirm-uploads limitation is irrelevant to this path. This confirms "remove Linear MCP, do attachments deterministically" — the maintainer's short-term "use Linear MCP" note predates having aws-samples#619 to copy.
…aws-samples#619) [P4.1] ADR-016 P4.1: Linear runs deterministically (no MCP), so the agent can't fetch uploads.linear.app images at runtime. New cdk/src/handlers/shared/linear-attachments.ts mirrors jira-attachments.ts: at webhook admission, fetch uploads.linear.app images (extracted from the issue description markdown) AUTHENTICATED with the workspace @bgagent OAuth bearer, validate magic bytes, screen through the same Bedrock Guardrail pipeline, upload cleaned bytes to S3, and return passed AttachmentRecords injected via createTaskCore's preScreenedAttachments seam. Fail-closed (LinearAttachmentError → reject task with a comment); SSRF-guarded (HTTPS-only, uploads-host-only, private-IP rejection); per-file + total + count caps on real bytes; batch cleanup of uploaded objects on any mid-batch failure. Wired into linear-webhook-processor single-task path (mints taskId up-front, fail-closed when ATTACHMENTS_BUCKET/GUARDRAIL unconfigured, cleanup on non-201). Public-CDN markdown images stay on the existing unauthenticated URL path; only uploads.linear.app comes through the authenticated path. +17 unit tests (linear-attachments.test.ts, 88.76% stmt cov). Updated the 2 webhook-processor tests that encoded the retired "skip uploads.linear.app" behavior to the new fetch/fail-closed contract. CDK build green: 163 suites/3230.
Extend linear-attachments.ts beyond PNG/JPEG to the full platform allowlist (PDF, text, csv, markdown, json, log). Linear attaches uploaded files as plain [label](uploads.linear.app/…) links (not inline images), so the description scan now matches both the image ![]() and link []() forms. MIME is resolved content-type → extension → magic-bytes (incl %PDF-), and non-image types are screened via screenTextFile, mirroring jira-attachments (aws-samples#619). Unsupported types (docx, zip, …) are silently skipped rather than failing the task; images/files/PDF still magic-byte validated and fail-closed. Adds file-type tests (PDF link, .log octet-stream, unsupported-skip).
…+ bundling) (ABCA-745) PDF attachment screening was silently broken on EVERY channel — live-caught when the Linear P4 path fetched a PDF and got 'PDF processing is unavailable'. Two independent bugs, both platform-wide (Jira aws-samples#619 equally affected): 1. API mismatch: code called pdf-parse as the v1 callable `pdfParseFn(buf)`, but the pinned pdf-parse@^2.4.5 exposes a `PDFParse` CLASS (`new PDFParse({data}).getText()`). A stale `@types/pdf-parse@^1` + a hand- written v1 module shim (src/types/pdf-parse.d.ts) made the wrong shape compile. Rewrote extractPdfText to the v2 class API (first:N page cap, destroy() teardown); removed both v1 type sources so v2's bundled types apply. 2. Bundling: the Linear + Jira webhook processors bundled pdf-parse with esbuild (only `externalModules:['@aws-sdk/*']`), mangling its pdfjs/@napi-rs/canvas deps → 'DOMMatrix is not defined' at import. Added `nodeModules:['pdf-parse']` so it resolves natively at runtime — matching the TaskApi/orchestrator attachment-screening bundling that already had the carve-out. Verified: PDFParse.getText extracts text headless in raw node24 (= the Lambda runtime). Tests mock the v2 class (ts-jest can't drive pdfjs's ESM worker); the real extraction is validated on the live deploy. text/csv/etc. unaffected.
…kips native canvas (ABCA-745) After fixing the v2 API + nodeModules bundling, PDF screening still failed live: pdfjs tried to load the native @napi-rs/canvas binding to supply DOMMatrix/ ImageData/Path2D, but the cross-platform binary isn't bundled for the Lambda's linux-arm64 (only the build host's darwin binary resolved) → 'Cannot find native binding' → 'DOMMatrix is not defined' → screening unavailable. Text extraction never actually uses canvas — pdfjs only touches those globals on its optional RENDER path. Defining them as inert no-op stubs BEFORE importing pdf-parse makes pdfjs skip the native-canvas load entirely and extract text headless — no native binary, host-independent. Verified: getText returns full text with canvas forced-absent + the three stubs present. This was platform-wide (confirm-uploads / Jira aws-samples#619 / Linear all fail-closed on every PDF) — PDF screening has been dead since the pdf-parse v2 bump.
3 participants
What
Closes #577. Jira-origin tasks now receive the same practical issue context Linear-origin tasks can use — attached files and recent comments, not just the summary/description. Because the Atlassian Remote MCP can't run headlessly (see the parent tracker #580 design note), the context is fetched authenticated at task-admission time in the webhook processor rather than on demand by the agent.
How
cdk/src/handlers/shared/jira-attachments.ts(new) —downloadScreenAndStoreJiraAttachments: resolves Jiramediaattachments via the gateway attachment-content endpoint (api.atlassian.com/ex/jira/{cloudId}/rest/api/3/attachment/content/{id}— the only host the 3LO token is valid against), refresh-and-retry-once on 401/403, enforces the size cap while streaming, validates magic bytes, screens each through the existing Bedrock Guardrail pipeline, uploads cleaned bytes to S3, and returnspassedAttachmentRecords. Fail-closed: a selected attachment that can't be safely fetched/screened throws and the task is rejected with a Jira comment. Unsupported/oversized/over-cap attachments are silently skipped. Filenames sanitized + ids validated to prevent S3-key / on-disk path traversal.fetchRecentHumanComments: recent human (accountType: atlassian) comments, ADF→markdown, oldest-first. Fail-open: any failure proceeds without them.cdk/src/handlers/shared/jira-adf.ts(new) — extracted the ADF→markdown walker so the processor and comment helper share it without a circular import (behavior-preserving).cdk/src/handlers/shared/create-task-core.ts—TaskCreationContextgains optionaltaskId(so processor-uploaded S3 keys match the eventual record) andpreScreenedAttachments(merged verbatim, never re-screened; a non-passedrecord fails closed).cdk/src/handlers/jira-webhook-processor.ts— wires both in; comments fold under a## Recent commentsheading, bounded so they never pushtask_descriptionpast the 10k limit (advisory context must not become a hard gate).cdk/src/constructs/jira-integration.ts+agent.ts— pass the attachments bucket to the processor, grant ReadWrite +ATTACHMENTS_BUCKET_NAME, bump the async processor timeout to 60s for serial download+screen. No new OAuth scopes (read:jira-workcovers reading attachments + comments).cdk/src/constructs/task-api.ts— exempt/v1/jira/webhookfrom the WAFSizeRestrictions_BODYmanaged rule (mirrors the existing Linear/GitHub webhook exemptions). Jira issue payloads carrying attachment metadata exceed the 8 KB body limit and were being blocked at the edge (403) before the receiver Lambda ran — so this is required for feat(jira): include Jira attachments and recent comments in task context #577 to function. The receiver still HMAC-verifies the raw body and the rate-limit rule still applies.Docs
docs/guides/JIRA_SETUP_GUIDE.mdgets an "Issue context: attachments and comments" section (supported types, limits, skip-vs-reject behavior, comment behavior) + regenerated Starlight mirror.Tests
jira-attachments.test.ts(new): download/filter/screen/upload, filename sanitization + unsafe-id rejection, 401 refresh-and-retry, size cap, magic-byte reject, screen-blocked → throw, comment human/app filtering + ADF→markdown + fail-open.jira-webhook-processor.test.ts: comment folding + truncation, fail-closed attachment rejection, remaining-slots math, no-attachments path.create-task-core.test.ts:preScreenedAttachmentsmerge without re-screening, caller-suppliedtaskId, non-passedfail-closed.task-api.test.ts: WAF exemption present in both the exclusion scope-down and the full-CRS scope-down.mise run buildgreen (cdk 2357 tests, cli 605, agent + docs pass, synth clean).Validated end-to-end
Deployed to a dev stack and triggered from a real Jira Cloud tenant: a SCRUM issue with 2 image attachments + a comment triggered a task → comments fetched → both attachments authenticated-downloaded, Bedrock-screened, and stored to S3 → agent hydrated them → committed the images → opened a PR. Confirmed the full path works.
Out of scope
Atlassian Remote MCP support; Confluence/project documents; fetching full historical comment threads; new attachment limits or file types beyond what ABCA already screens.